Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the URL label to metrics securely #966

Closed
wants to merge 1 commit into from

Conversation

danlsgiga
Copy link
Contributor

Small feature addition.

Sometimes is useful to have the endpoint URL in metrics labels so we can include that as part of alert messages when these metrics are scraped via prometheus.

This is related to #875 with the requested addition of sanitizing the URL to prevent query parameters, which may contain secrets, from leaking to metrics.

Additionaly, added a new test to validate query parameters are not being exposed.

@danlsgiga
Copy link
Contributor Author

@TwiN I hope this covers the issue raised in the previous PR. 🙌🏻

@danlsgiga
Copy link
Contributor Author

I believe the proposed feature in #979 is a really nice addition and can make this PR obsolete. Let me know what you think @TwiN

@appleboy
Copy link
Contributor

appleboy commented Jan 29, 2025

Thanks, @danlsgiga reference the PR

@TwiN I have added a customizable label feature that covers the functionality of this pull request.

You can view the pull request here.

@TwiN
Copy link
Owner

TwiN commented Jan 29, 2025

Looking at #979 now 👀

For #966, if we end up merging this as well, should we take ui.hide-hostname into consideration, since technically the metrics endpoint isn't protected out of the box? Especially since the metrics endpoint is exposed under the same port as Gatus' UI

@danlsgiga
Copy link
Contributor Author

I'd go with @appleboy implementation which is much more flexible. Feel free to close this PR and get #979 reviewed / merged as that will also help us on other use cases.

@appleboy
Copy link
Contributor

@danlsgiga

You can close this PR yourself. Let's continue the discussion in #979, and please help review it as well.

@TwiN
Copy link
Owner

TwiN commented Jan 30, 2025

If you prefer the label implementation, I'm fine with that!

I do think the label implementation has a lot of perks, such as perhaps making exposing these labels on the UI as "tags" of sorts.

@TwiN TwiN closed this Jan 30, 2025
@danlsgiga danlsgiga deleted the baseurl-on-metric-labels branch January 30, 2025 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants